Provide external jackson object mapper #395
Conversation
|
@SlyngDK Please review. |
runtime/src/test/java/io/quarkiverse/loggingjson/providers/JsonProviderBaseTest.java
Show resolved
Hide resolved
runtime/src/test/java/io/quarkiverse/loggingjson/providers/JsonProviderBaseTest.java
Show resolved
Hide resolved
runtime/src/test/java/io/quarkiverse/loggingjson/providers/JsonProviderBaseTest.java
Show resolved
Hide resolved
runtime/src/test/java/io/quarkiverse/loggingjson/providers/JsonProviderBaseTest.java
Show resolved
Hide resolved
# dependency-version: 0.8.14 # dependency-type: direct:development # update-type: version-update:semver-patch # dependency-version: 0.8.14 # dependency-type: direct:production # update-type: version-update:semver-patch # dependency-version: 3.29.0 # dependency-type: direct:production # update-type: version-update:semver-minor # dependency-version: 3.29.0 # dependency-type: direct:production # update-type: version-update:semver-minor # dependency-version: 3.29.0 # dependency-type: direct:production # update-type: version-update:semver-minor # dependency-version: 3.29.0 # dependency-type: direct:production # update-type: version-update:semver-minor # dependency-version: 3.29.0 # dependency-type: direct:production # update-type: version-update:semver-minor # dependency-version: 3.29.0 # dependency-type: direct:development # update-type: version-update:semver-minor #dependency-version: 3.29.0 # dependency-type: direct:development
IvanPuntev
force-pushed
the
provide-external-jackson-object-mapper
branch
from
099fc28 to
1f915cc
Compare
|
@SlyngDK Should we use a single json factory instance instead of creating new one for each build step? |
gsmet
left a comment
gsmet
left a comment
There was a problem hiding this comment.
Had a quick look and spotted a few things.
| <dependency> | ||
| <groupId>org.eclipse</groupId> | ||
| <artifactId>yasson</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
Why are you enforcing Yasson now? It doesn't look like an improvement?
There was a problem hiding this comment.
The jsonb factory initialization is now moved from the processor (deployment) to the recorder (runtime) and it is instantiated only on demand. The dependency is added because the native build starts to break without it. It is not present in the runtime classpath unless you explicitly depend on it.
Error: Discovered unresolved type during parsing: org.eclipse.yasson.internal.JsonBindingBuilder. This error is reported at image build time because class io.quarkiverse.loggingjson.jsonb.JsonbJsonFactory is registered for linking at image build time by command line and command line.
runtime/src/main/java/io/quarkiverse/loggingjson/jackson/JacksonJsonFactory.java
Outdated
Show resolved
Hide resolved
| @Singleton | ||
| public class ObjectMapperProvider { | ||
|
|
||
| @Inject | ||
| ObjectMapper mapper; | ||
|
|
||
| public ObjectMapper get() { | ||
| return this.mapper; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
I doubt this will work well in practice as you want logging very early, even before ArC is set up.
If you require ArC to be set up to set up logging, it might work but you will collect lots of messages and buffer them in memory before the setup is fully done.
I think you probably need to use the ServiceLoader for this and my guess is that you won't be able to reuse the common ObjectMapper.
There was a problem hiding this comment.
The idea is to be able to get the object mapper provided from the project including the extension. Don't think the ServiceLoader will do...
There was a problem hiding this comment.
Arc usage should be ok since it is already used in the same recorder https://github.com/quarkiverse/quarkus-logging-json/blob/main/runtime/src/main/java/io/quarkiverse/loggingjson/LoggingJsonRecorder.java#L57 Also the processor build steps are marked as @record(ExecutionTime.RUNTIME_INIT)
| final Config.FieldConfig config = fieldConfig(Optional.empty(), Optional.empty()); | ||
| final Config.FieldConfig config = fieldConfig(null, null); |
There was a problem hiding this comment.
I think it's not a very good idea to mix changes that are not really related to the issue at hand. It makes the PR very verbose.
There was a problem hiding this comment.
I'm aware of that but I considered these changes harmless and since there is no frequent activity in this repo decided to include them.
3 participants
Fix #242
Fix #305
Obsolete #306